Skip to content

[iceberg] Add metrics reporter for iceberg table scans #24904

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[iceberg] Add metrics reporter for iceberg table scans #24904

wants to merge 1 commit into from
+296 −31

Conversation

j-sund
Copy link

@j-sund j-sund commented Apr 10, 2025 •
edited
Loading

Description

Adds a new MetricsReporter implementation to propagate Iceberg table scan metrics into session RuntimeStats

Motivation and Context

Exposing these metrics can improve query observability, which thus makes future improvements to the Iceberg integration a little easier. This also enables better performance monitoring.

Impact

Allows for better visibility into the Iceberg integration.

Test Plan

Ensure the metric is visible and being populated. Basic tests are included in IcebergDistributedSmokeTestBase.java class

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • [] Adequate tests were added if applicable.
  • CI passed.

Release Notes

== NO RELEASE NOTE ==

@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Apr 10, 2025 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: j-sund / name: Joshua Sundararaman (55e21d8)

@j-sund j-sund marked this pull request as ready for review April 12, 2025 03:16
@j-sund j-sund requested review from hantangwangd, ZacBlanco and a team as code owners April 12, 2025 03:16
@j-sund j-sund requested a review from presto-oss April 12, 2025 03:16
@ZacBlanco
Copy link
Contributor

@j-sund Can you sign the CLA using the link in the comment above? You can just sign as a personal contributor.

Also, before your next push try running ./mvnw validate -pl presto-iceberg to ensure your code passes all the checkstyle and license rules. I see the maven checks / maven-checks CI action is failing with the following:

Warning:  Missing header in: /home/runner/work/presto/presto/presto-iceberg/src/main/java/com/facebook/presto/iceberg/RuntimeStatsMetricsReporter.java

@j-sund j-sund force-pushed the runtimeStatsReporter branch from cfc3890 to 67e5b9b Compare April 23, 2025 16:42
ZacBlanco
ZacBlanco requested changes Apr 30, 2025
View reviewed changes
Copy link
Contributor

@ZacBlanco ZacBlanco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just minor things. This looks great!

@j-sund j-sund force-pushed the runtimeStatsReporter branch 2 times, most recently from 72ef7e5 to 6124180 Compare May 5, 2025 21:42
ZacBlanco
ZacBlanco requested changes May 5, 2025
View reviewed changes
@j-sund j-sund force-pushed the runtimeStatsReporter branch 2 times, most recently from 5b96c26 to 8b19e78 Compare May 16, 2025 18:50
hantangwangd
hantangwangd requested changes May 17, 2025
View reviewed changes
Copy link
Member

@hantangwangd hantangwangd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a great feature! Some nits, and a minor problem in the test code.

@hantangwangd hantangwangd mentioned this pull request May 22, 2025
Merged
6 tasks
@hantangwangd
Copy link
Member

I have submit a PR #25140 to make sure that the table name loaded from all catalogs have consistent formatting <catalog>.<schema>.<table>. So I think after that change being merged, we can use the following code to build table name uniformly:

        String catalog = getSession().getCatalog().get();
        String schema = getSession().getSchema().get();
        String tableName = catalog + "." + schema + ".orders";

@hantangwangd
Copy link
Member

Hi @j-sund, since PR #25140 has been merged, you can rebase to include the change. This enables a consistent way to build the table name as described above.

@j-sund j-sund force-pushed the runtimeStatsReporter branch 2 times, most recently from 8e4ba6a to 9ef6b65 Compare June 5, 2025 05:06
@j-sund j-sund requested a review from ZacBlanco June 5, 2025 14:00
ZacBlanco
ZacBlanco requested changes Jun 5, 2025
View reviewed changes
Copy link
Contributor

@ZacBlanco ZacBlanco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few style nits. Otherwise LGTM! Can you also squash all of your commits into one?

ZacBlanco
ZacBlanco reviewed Jun 6, 2025
View reviewed changes
Copy link
Contributor

@ZacBlanco ZacBlanco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Can you please squash all of your commits into one? Also, try to write a descriptive commit title+message

@j-sund j-sund force-pushed the runtimeStatsReporter branch from bdc7f4b to ff80c23 Compare June 6, 2025 22:22
@j-sund j-sund changed the title RuntimeStatsMetric Reporter for table scans in Iceberg Connector [iceberg] Add metrics reporter for iceberg table scans Jun 6, 2025
@j-sund j-sund force-pushed the runtimeStatsReporter branch from ff80c23 to 55e21d8 Compare June 6, 2025 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZacBlanco ZacBlanco ZacBlanco requested changes

@hantangwangd hantangwangd hantangwangd requested changes

@presto-oss presto-oss Awaiting requested review from presto-oss presto-oss is a code owner automatically assigned from prestodb/committers

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@j-sund @ZacBlanco @hantangwangd